Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: custom end meeting url with restriction #202

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

SpechtD
Copy link
Contributor

@SpechtD SpechtD commented Mar 16, 2022

closes #162

Copy link
Member

@sualko sualko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but needs some changes until we can merge it.

lib/Controller/RestrictionController.php Show resolved Hide resolved
lib/Controller/RoomController.php Outdated Show resolved Hide resolved
lib/Migration/Version000000Date20220316125900.php Outdated Show resolved Hide resolved
lib/Service/RestrictionService.php Outdated Show resolved Hide resolved
lib/Service/RestrictionService.php Outdated Show resolved Hide resolved
lib/Service/RoomService.php Outdated Show resolved Hide resolved
@@ -119,7 +119,8 @@ public function update(
bool $listenOnly,
bool $mediaCheck,
bool $cleanLayout,
bool $joinMuted
bool $joinMuted,
string $logoutURL
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need some type of validation for the logoutURL. At least it has to start with http:// or https://.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This validation should also be done on the server side, otherwise an attacker could bypass the client logic.

@@ -137,13 +138,17 @@ public function update(
return new DataResponse(['message' => 'Not allowed to enable recordings.'], Http::STATUS_BAD_REQUEST);
}

if (!$restriction->getallowLogoutURL() && $logoutURL !== $room->getlogoutURL()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the value of allowLogoutURL determined somewhere? You should probably look into the Permission class 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value of allowLogoutURL is determined in the Restriction class, together with the other restrictions.


$restriction1 = new Restriction();
$restriction1->setRoomTypes(\json_encode([Room::ACCESS_INTERNAL, Room::ACCESS_INTERNAL_RESTRICTED]));
$restriction1->setMaxRooms(10);
$restriction1->setMaxParticipants(100);
$restriction1->setAllowRecording(true);
$restriction1->setAllowRecording(true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably setAllowLogoutURL.

@@ -119,7 +119,8 @@ public function update(
bool $listenOnly,
bool $mediaCheck,
bool $cleanLayout,
bool $joinMuted
bool $joinMuted,
string $logoutURL
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This validation should also be done on the server side, otherwise an attacker could bypass the client logic.

@@ -188,6 +188,10 @@ private function buildMeetingParams(Room $room, Presentation $presentation = nul
$invitationUrl = $this->urlHelper->linkToInvitationAbsolute($room);
$createMeetingParams->setModeratorOnlyMessage($this->l10n->t('To invite someone to the meeting, send them this link: %s', [$invitationUrl]));

if (!empty($room->logoutURL)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably check if the logoutURL is empty. Anyway I would prefer to have the assignment at one place. Maybe

$createMeetingParams->setLogoutURL($room->logoutURL || $this->urlGenerator->getBaseUrl());

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom end meeting url
2 participants